-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update resolver to query user by orcid or id #3258
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3258 +/- ##
==========================================
- Coverage 46.68% 46.67% -0.01%
==========================================
Files 609 609
Lines 38701 38734 +33
Branches 1225 1232 +7
==========================================
+ Hits 18068 18080 +12
- Misses 20438 20459 +21
Partials 195 195 ☔ View full report in Codecov by Sentry. |
@nellh - I think I have this as we discussed. I will need some help to write test coverage for this also I added the gql user query in the user-query.tsx and I am writing/updating tests for that. I will also need to adjust templates to account for fields that are not required (like avatar). Putting back into draft but would love your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this looks good. The precommit hook didn't get run so the formatting is wrong in some spots. One fix needed for the functionality but it would also be good to fix the formatting.
} | ||
|
||
if (isValidOrcid(id)) { | ||
return User.findOne({ "orchid": id }).exec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return User.findOne({ "orchid": id }).exec() | |
return User.findOne({ "orcid": id }).exec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo here but another limitation is there's two kinds of user records here:
{
"provider": "orcid",
"providerId": "0000-0000..."
}
{
"provider": "google",
"orcid": "0000-0000..."
}
This branch needs to find both of these. Once we've migrated Google users to ORCID logins, most users will fall into the first group that won't match here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nellh something like this?
export const user = (obj, { id, key }) => {
function isValidOrcid(orcid: string): boolean {
return /^[0-9]{4}-[0-9]{4}-[0-9]{4}-[0-9]{3}[0-9X]$/.test(orcid || "")
}
if (isValidOrcid(id)) {
if (key === 'orcid') {
return User.findOne({ "orcid": id }).exec(); // Search by orcid if key is 'orcid'
} else {
return User.findOne({ "providerId": id }).exec(); // Otherwise, search by providerId
}
} else {
return User.findOne({ "id": id }).exec(); // Search by id if not a valid ORCID
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a single query, you can use the {$or: [{ "orcid": id }, {"providerId": id}]}
operator to check if the id is in either field. No need for a key argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty - that was helpful. I have that added now
updating the resolver so a user can be queried by orcid or id.